HPCC-32795 Additional optimizations to replaceString #19239
Conversation
- Does nothing if no search string is supplied or if it is larger than the source string - Compare directly if search and source string are equal in length Signed-off-by: Jack Del Vecchio <[email protected]>
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32795 Jirabot Action Result: |
system/jlib/jstring.cpp
Outdated
| if (oldlen == curLen) | ||
| { | ||
| if (memcmp(buffer, oldStr, oldlen) == 0) | ||
| temp.append(newlen, newStr); |
There was a problem hiding this comment.
I would think this would be more efficient to copy newStr directly into buffer, rather than going through temp.
If you make that change, then temp and the call to swap() can be moved to the same block.
There was a problem hiding this comment.
I agree it would be more efficient to copy directly and only use the temp buffer when necessary.
- Add unit tests for special case
There was a problem hiding this comment.
This PR only partially implements the suggestions in the jira ticket.
- No replacements take place
This covers the case where the text is smaller than the search text, but does not cover the more general case where there were matches.
Hint: What parameter could be passed, and how could a common function efficiently interpret it to avoid a memcpy and memory allocation if no search string was found?
- The source and target replacement strings are the same length
You have implement the source and current string are the same length.
Hint: How can you optimize and in-place replace if the source and target strings are the same length?
There is actually a 3rd case:
- The replacement text is shorter than the search text
Question: What optimization does this enable for an in-place replacement? Can you efficiently common this up with case (2) to avoid duplicating too much code.
| { | ||
| if (memcmp(buffer, oldStr, oldlen) == 0) | ||
| { | ||
| memcpy(buffer, newStr, newlen); |
There was a problem hiding this comment.
If I was reviewing this code (I am not because of general review comments) - this has a serious memory corruption if newlen > oldlen
There was a problem hiding this comment.
Fixed. I added ensureCapacity(newlen).
- Add ensureCapacity(newlen)
- Add additional parameter to replaceString that returns whether a match was found - Wait to allocate memory until a match is made rather than at the beginning of the search - Avoid copying source into result if source wasn't changed
- Move the target into the source rather than copy and skip allocating. - Add tests for each case of target length
system/jlib/jstring.cpp
Outdated
| while (offset < maxOffset) | ||
| { | ||
| if (unlikely(source[offset] == firstChar) | ||
| && unlikely((lenOldStr == 1) || memcmp(source + offset, oldStr, lenOldStr)==0)) | ||
| { | ||
| // Wait to allocate memory until a match is found | ||
| if (unlikely(!foundMatch)) |
There was a problem hiding this comment.
@ghalliday I am not sure I got this correct. I chose unlikely because it will only ever be true on the first match, so any string with multiple matches will get some benefit. If this is rare and in most cases there is only a single match then it should probably be likely. Is that correct?
There was a problem hiding this comment.
Most of the time I wouldn't worry about adding likely/unlikely. The case where it is may be important is in inner loops that are the critical points. Once we have a match it is much less critical.
|
@ghalliday I believe I got all the optimizations you were asking for.
Let me know if I got any of these wrong. Back to you. |
rtl/eclrtl/eclrtl.cpp
Outdated
| ::replaceString(result, rtlUtf8Size(scriptChars, script), script, rtlUtf8Size(searchChars, search), search, rtlUtf8Size(outFieldsChars, outFields), outFields); | ||
| bool foundMatch = false; | ||
| size_t sourceLen = rtlUtf8Size(scriptChars, script); | ||
| ::replaceString(result, sourceLen, script, rtlUtf8Size(searchChars, search), search, rtlUtf8Size(outFieldsChars, outFields), outFields, foundMatch); |
There was a problem hiding this comment.
if this is the only place replaceString is called then I would consider a slightly different approach.
i) Pass a flag to indicate whether to copy if there is no match
ii) change the return type to be a boolean and return whether or not a replace occurred.
otherwise it is a slightly strange semantics for a pubic function.
There was a problem hiding this comment.
Changed to return a bool indicating if a match was found and takes a parameter that sets whether to copy regardless of a match being found.
system/jlib/jstring.cpp
Outdated
| while (offset < maxOffset) | ||
| { | ||
| if (unlikely(source[offset] == firstChar) | ||
| && unlikely((lenOldStr == 1) || memcmp(source + offset, oldStr, lenOldStr)==0)) | ||
| { | ||
| // Wait to allocate memory until a match is found | ||
| if (unlikely(!foundMatch)) |
There was a problem hiding this comment.
Most of the time I wouldn't worry about adding likely/unlikely. The case where it is may be important is in inner loops that are the critical points. Once we have a match it is much less critical.
- Returns a bool indicating whether a match was made - Accepts a parameter to force a copy even if no match was found
Type of change:
Checklist:
Smoketest:
Testing: